feat(cross-subname-indexing): allow activating multiple plugins together by tk-o · Pull Request #21 · namehash/ensnode

tk-o · 2025-01-12T11:43:40Z

This PR allows cross-subname indexing. For example. with this change, it's possible to get a list of all names a given address owns across multiple subnames (i.e. base.eth, linea.eth).

Example query

query GetOwnedDomains($ownerAddreess: String!) {
  domainss(
    where: {OR: [{ownerId: $ownerAddreess}, {wrappedOwnerId: $ownerAddreess}]}
  ) {
    items {
      labelName
      name
      ownerId
      wrappedOwnerId
      createdAt
      expiryDate
    }
  }
}

Preview

Domain names under eth, base.eth, and linea.eth:

tk-o · 2025-01-12T11:44:56Z

      await context.db
        .update(domains, { id: node })
-        .set({ labelName: name, name: `${name}${ownedName}` });
+        .set({ labelName: name, name: `${name}.${ownedName}` });


This was a bug, that existed since we dropped . from the ownedName param on plugin's configuration object.

Good catch, thanks.

tk-o · 2025-01-12T11:46:26Z

+      createdAt: 0n,
+      isMigrated: false,
+    })
+    .onConflictDoNothing();


I had to introduce .onConflictDoNothing() as multiple plugins run the setupRootNode() function, so every call apart from the initial one would cause an error.

@tk-o Hey really appreciate your PR comment on this! Could you please also document this idea on each place in the code where this idea applies? This way the relevant documentation will live on in our code in an easy to find place after this PR is merged.

Taking a step-back, I'm a bit confused on why there might be conflicts caused by other plugins here. I suppose the conflict might be caused by how we are enabling the base.eth or linea.eth plugins to be activated without the .eth plugin being activated. In other words, we don't have any concept of plugin dependencies implemented, such that the activation of plugin X also requires the activation of plugin Y.

Do I understand that correctly? Appreciate your advice.

I'm ok to not introduce plugin dependencies yet. I think we can come back to this as a separate goal in the future. It would add complexity and I'm not sure if its even the best idea. For now just trying to understand the current situation better. Thanks 👍

Each plugin requires a root node to be included in the domains collection so that all TLDs could be attached the parent node (the root node). Each plugin needs the root domain entry to be added before the rest of indexing happens. No plugin can expect any other plugin to do that job, so each plugin tries to do it on its own. If multiple plugins are activated, the only one of them will effectively insert the root domain entry into the domains table, while other plugins will do nothing.

shrugs · 2025-01-12T16:53:39Z

+const activePlugins = loadActivePlugins();

-  pluginToActivate.activate();
+export default (() =>


can you just return createConfig(loadActivePlugins()) as AllPluginsConfig? feels like spreading contracts: {...activePlugins.contracts} isn't doing much?

Thanks for this suggestion, now it looks way cleaner ;)

shrugs · 2025-01-12T16:55:44Z

+ * const obj3 = deepMergeRecursive(obj1, obj2);
+ * // { a: 4, b: 2, c: { d: 3, e: 5 } }
+ */
+export function deepMergeRecursive<T extends AnyObject, U extends AnyObject>(


i'd actually prefer to depend on a package for this, what do you think? no dependency is nice but this looks complex enough and generic enough that a well-tested dependency seems a-ok to me. i found this one for example https://github.com/voodoocreation/ts-deepmerge

We could use lib, I tried ts-deepmerge and deepmerge — couldn't get the types to work easily, so gave up. On the other hand, this one works well and does its job.

@tk-o Thanks for proposing this. Agreed with @shrugs that a well-tested library for this sounds attractive. My head hurts thinking for code review of testing on this.

Appreciate if it might take some time to figure out how to get one of those libraries to work. If we can find a solution to that in this PR, then great 👍 If it needs more time, then please feel welcome to create a new GitHub Issue for us to track this goal and we can come back to it in a separate PR.

tk-o · 2025-01-12T19:30:22Z

+    if (await incrementSubdomainCountOnParent()) {
+      await context.db
+        .update(domains, { id: parent!.id })
+        .set((row) => ({ subdomainCount: row.subdomainCount + 1 }));
+    }


@shrugs, I hope you'll like this change. It allows not updating the parent domain entry if it does not exist. Useful during partial block-range indexing, and neutral to full block-range indexing.

@tk-o Hey thanks for your PR comment above. I see that comment is specific to lines 135-139. What about all the other changes being proposed to handleNewOwner in this PR? Can you please help us understand the bigger goal for these changes?

While we're working on our "V1" milestone I'm cautious about changes to indexing logic that are difficult for me to easily review. Open to making them if there's a good reason 👍

lightwalker-eth

@tk-o Super exciting updates in this PR! 🚀 Reviewed and shared feedback.

lightwalker-eth · 2025-01-13T09:52:27Z

+      /**
+       * Address the issue where in the same transaction the Transfer event occurs before the NameRegistered event.
+       * Example: https://lineascan.build/tx/0x2211c5d857d16b7ac111088c57fb346ab94049cb297f02b0dda7aaf4c14d305b#eventlog
+       * Code: hhttps://github.com/Consensys/linea-ens/blob/main/packages/linea-ens-contracts/contracts/ethregistrar/BaseRegistrarImplementation.sol#L155-L160


Suggested change

* Code: hhttps://github.com/Consensys/linea-ens/blob/main/packages/linea-ens-contracts/contracts/ethregistrar/BaseRegistrarImplementation.sol#L155-L160

* Code: https://github.com/Consensys/linea-ens/blob/main/packages/linea-ens-contracts/contracts/ethregistrar/BaseRegistrarImplementation.sol#L155-L160

lightwalker-eth · 2025-01-13T09:56:36Z

  ponder.on(pluginNamespace("BaseRegistrar:Transfer"), async ({ context, event }) => {
+    if (event.args.from === zeroAddress) {
+      /**
+       * Address the issue where in the same transaction the Transfer event occurs before the NameRegistered event.


Really appreciate the comment here! Very happy to see 👍

Could you please add even more background info into the comment here?

For example:

Exactly what is causing the Transfer event to occur before the NameRegistered event?

How does this relate to the event.args.from being the zeroAddress?

Please describe the big idea behind what this new code is doing? What does it represent in the big picture? I see we're inserting some new domain, but it isn't clear why or what we're trying to achieve.

The contract emits the Transfer event before emitting the NameRegistered event. It's how the contract implementation is done. Our BaseRegistrar:Transfer handler previously assumed that the domain name would already be included in the domains table, but that's not always the case. Registering a new subname first transfers an NFT token from the zero address to the owner, and then emits the NameRegistered event.

Minting the subname NFT is effectively transferring it from the zero address to the owner.

This code is ensuring that the BaseRegistrar:NameRegistered handler can assume that the domain name has been for sure present in the database.

lightwalker-eth · 2025-01-13T09:59:21Z

@@ -46,7 +47,28 @@ export default function () {

  // Base's BaseRegistrar uses `id` instead of `tokenId`


Suggested change

// Base's BaseRegistrar uses `id` instead of `tokenId`

// base.eth's BaseRegistrar uses `id` instead of `tokenId`

lightwalker-eth · 2025-01-13T09:59:51Z

  // Base's BaseRegistrar uses `id` instead of `tokenId`
  ponder.on(pluginNamespace("BaseRegistrar:Transfer"), async ({ context, event }) => {
-    return await handleNameTransferred({
+    if (event.args.from === zeroAddress) {


Please see related feedback I shared for this on related code for linea.eth

lightwalker-eth · 2025-01-13T10:18:53Z

+      createdAt: 0n,
+      isMigrated: false,
+    })
+    .onConflictDoNothing();


@tk-o Hey really appreciate your PR comment on this! Could you please also document this idea on each place in the code where this idea applies? This way the relevant documentation will live on in our code in an easy to find place after this PR is merged.

Taking a step-back, I'm a bit confused on why there might be conflicts caused by other plugins here. I suppose the conflict might be caused by how we are enabling the base.eth or linea.eth plugins to be activated without the .eth plugin being activated. In other words, we don't have any concept of plugin dependencies implemented, such that the activation of plugin X also requires the activation of plugin Y.

Do I understand that correctly? Appreciate your advice.

I'm ok to not introduce plugin dependencies yet. I think we can come back to this as a separate goal in the future. It would add complexity and I'm not sure if its even the best idea. For now just trying to understand the current situation better. Thanks 👍

lightwalker-eth · 2025-01-13T10:22:08Z

-// relevant config is run at runtime
-export default ((): AllConfigs => {
-  const pluginToActivate = plugins.find((p) => p.ownedName === ACTIVE_PLUGIN);
+// The type of the exported default is the intersection of all plugin configs to


A few questions:

What is "the type of the exported default"?

When you write "all plugin configs" or AllPluginsConfig, is this all activated plugins, or all available plugins? Appreciate if you could clarify terminology.

I should say "the type of the default export"

"all plugins config" mean an intersection type of the available plugins (regardless if active or not). This type is required to support all defined indexing handlers type-safety.

lightwalker-eth · 2025-01-13T10:22:46Z

 import * as ethPlugin from "./src/plugins/eth/ponder.config";
 import * as lineaEthPlugin from "./src/plugins/linea.eth/ponder.config";

 const plugins = [baseEthPlugin, ethPlugin, lineaEthPlugin] as const;


Suggested change

const plugins = [baseEthPlugin, ethPlugin, lineaEthPlugin] as const;

const availablePlugins = [baseEthPlugin, ethPlugin, lineaEthPlugin] as const;

Suggesting this to help us give more distinct naming to all available plugins vs all activated plugins.

lightwalker-eth · 2025-01-13T10:24:42Z

      await context.db
        .update(domains, { id: node })
-        .set({ labelName: name, name: `${name}${ownedName}` });
+        .set({ labelName: name, name: `${name}.${ownedName}` });


Good catch, thanks.

lightwalker-eth · 2025-01-13T10:32:22Z

+    if (await incrementSubdomainCountOnParent()) {
+      await context.db
+        .update(domains, { id: parent!.id })
+        .set((row) => ({ subdomainCount: row.subdomainCount + 1 }));
+    }


@tk-o Hey thanks for your PR comment above. I see that comment is specific to lines 135-139. What about all the other changes being proposed to handleNewOwner in this PR? Can you please help us understand the bigger goal for these changes?

While we're working on our "V1" milestone I'm cautious about changes to indexing logic that are difficult for me to easily review. Open to making them if there's a good reason 👍

shrugs · 2025-01-13T16:51:53Z

+ * @returns the active plugins
+ */
+export function getActivePlugins<T extends { ownedName: string }>(plugins: readonly T[]): T[] {
+  const pluginsToActivateByOwnedName = ACTIVE_PLUGIN


let's rename this to ACTIVE_PLUGINS now that it's a plural array, and make sure to update .env.local.example as well

tk-o · 2025-01-13T20:11:29Z

Closing this PR in favour of:

feat(cross-chain-indexing): enable multiple concurrent plugins #24

feat(cross-subname-indexing): allow activating multiple plugins together

618e8d3

tk-o commented Jan 12, 2025

View reviewed changes

Comment thread src/plugins/base.eth/ponder.config.ts Outdated

feat(cross-subname-indexing): demonstrate retrieving cross-subname index

7d8f978

tk-o force-pushed the feat/cross-subname-indexing branch from 720ed9f to 7d8f978 Compare January 12, 2025 11:54

shrugs reviewed Jan 12, 2025

View reviewed changes

tk-o added 2 commits January 12, 2025 20:20

refactor(handlers): stremline logic

76754e4

refactor(ponder.config): simplify structure

0ffa78c

tk-o commented Jan 12, 2025

View reviewed changes

lightwalker-eth requested changes Jan 13, 2025

View reviewed changes

fix: apply pr feedback

b118dfb

shrugs reviewed Jan 13, 2025

View reviewed changes

tk-o added 3 commits January 13, 2025 17:55

fix: typos

c71479f

fix: apply pr feedback

ceb03cc

feat(plugins): introduce ACTIVE_PLUGINS validation

fa57a34

tk-o mentioned this pull request Jan 13, 2025

feat(cross-chain-indexing): enable multiple concurrent plugins #24

Merged

tk-o closed this Jan 13, 2025

tk-o deleted the feat/cross-subname-indexing branch January 15, 2025 06:25

	* Code: hhttps://github.com/Consensys/linea-ens/blob/main/packages/linea-ens-contracts/contracts/ethregistrar/BaseRegistrarImplementation.sol#L155-L160
	* Code: https://github.com/Consensys/linea-ens/blob/main/packages/linea-ens-contracts/contracts/ethregistrar/BaseRegistrarImplementation.sol#L155-L160

		@@ -46,7 +47,28 @@ export default function () {

		// Base's BaseRegistrar uses `id` instead of `tokenId`

	// Base's BaseRegistrar uses `id` instead of `tokenId`
	// base.eth's BaseRegistrar uses `id` instead of `tokenId`

	const plugins = [baseEthPlugin, ethPlugin, lineaEthPlugin] as const;
	const availablePlugins = [baseEthPlugin, ethPlugin, lineaEthPlugin] as const;

Uh oh!

Conversation

tk-o commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example query

Preview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lightwalker-eth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tk-o Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tk-o commented Jan 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tk-o commented Jan 12, 2025 •

edited

Loading

tk-o Jan 13, 2025 •

edited

Loading